Add timeout for udp dns requests to avoid resource leak - #41395
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential resource leak in the Linux-side DNS tunneling path by ensuring UDP DNS requests don’t remain tracked indefinitely when the Windows side never produces a response. It does this by introducing per-request expiration tracking and using epoll timeouts to periodically reap stale requests.
Changes:
- Adds per-UDP-request expiration tracking (request ID → remote address + expiration iterator) and an ordered expiration queue.
- Implements
ExpireUdpRequestsAndGetTimeout()to purge expired requests and driveepoll_wait()with a dynamic timeout. - Updates UDP response handling to remove both the request-map entry and its corresponding expiration entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/linux/init/DnsServer.h | Adds UDP request context + expiration queue members and helper declaration. |
| src/linux/init/DnsServer.cpp | Implements request expiration logic and integrates epoll timeout + cleanup on success/error paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/linux/init/DnsServer.cpp:420
- The request id is assumed to be reusable after wraparound, but with a 60s in-flight window it’s possible to collide with an existing entry. On collision the current logic logs an error/throws and drops the request. Prefer selecting an unused id (retry) so requests keep flowing under high load.
const auto expiration = std::chrono::steady_clock::now() + c_udpRequestTimeout;
const auto expirationIt = m_udpRequestExpirations.emplace(m_udpRequestExpirations.end(), expiration, requestId);
auto removeExpirationOnError = wil::scope_exit([&] { m_udpRequestExpirations.erase(expirationIt); });
const auto [_, inserted] = m_udpRequests.emplace(requestId, UdpRequestContext{remoteAddr, expirationIt});
src/linux/init/DnsServer.h:120
- This member comment still says the map stores a
sockaddr_in, but the value is nowUdpRequestContext(remote address + expiration iterator). Updating the comment will avoid confusion for future changes.
// Mapping id of an UDP DNS request to the sockaddr_in struct storing the IP and port used by the Linux DNS client that made
// the DNS request. Note: Since we only configure an IPv4 DNS server in Linux, we expect all Linux DNS clients to use IPv4
// addresses. _Guarded_by_(m_udpLock)
std::map<uint32_t, UdpRequestContext> m_udpRequests;
Blue (OneBlue)
left a comment
Blue (OneBlue)
left a comment
There was a problem hiding this comment.
Indeed looking through DnsResolver::HandleDnsQueryCompletion it's looking like there are code paths where we don't actually send an error back to the client.
Instead of handling failures on the client side through, I think a better path would be to return an error response to the linux side so it can cleanup its resources. That way the timeout is 100% driven by Windows.
Approving since the current change is an improvement over the current behavior
Summary of the Pull Request
The tunneled UDP DNS request could leak if the Windows side produces no response.
This PR adds a 60s timeout for each request to avoid this potential leak.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Tested manually:
Before the fix:
5. Waited over 70 seconds and confirmed the map remained above 1,000 and continued growing.
After the fix:
5. Confirmed requests were removed approximately 60 seconds after insertion.